Thread detach hook for Java JNI on Android #250

johnno1962 · 2017-05-23T00:10:58Z

This is a callback hook required when mixing multi threaded Swift and Java on Android. When Java JNI has been used on a thread the thread needs to be detached from JNI before the thread exits to free local variables or the application will crash e.g. https://github.com/SwiftJava/java_swift/blob/master/Sources/JavaJNI.swift#L20.
Introducing this hook gives any future Java integration code a chance to do this.

MadCoder · 2017-05-23T02:36:30Z

dispatch/queue.h

+ * "detached" before the thread exits or the application will crash.
+ */
+DISPATCH_EXPORT
+void (*dispatch_thread_detach_handler)();


this should go to queue_internal.h

this should be (void)

this should only be enabled on android

Changes made. queue_interal.h doesn’t seem to be visible from Swift so I have left it where it is and prefixed the symbol with an “_" for now.

MadCoder · 2017-05-23T02:37:21Z

src/queue.c

@@ -861,6 +861,8 @@ gettid(void)
 		if ((f) && tsd->k) ((void(*)(void*))(f))(tsd->k); \
 	} while (0)

+void (*dispatch_thread_detach_handler)();


Should be on android only

MadCoder · 2017-05-26T18:20:53Z

dispatch/queue.h

+ * "detached" before the thread exits or the application will crash.
+ */
+DISPATCH_EXPORT
+void (*_dispatch_thread_detach_callback)(void);


for the second time this is NOT appropriate for an API header, this belongs to an internal one.
if the swift overlay is not built seeing the internal headers, then either try to fix that, or extern the prototype here again. it is an internal implementation detail between the overlay and libdispatch that should never be exposed anywhere.

this HAS to move to queue_internal.h, or maybe "private/private.h" this could be appropriate as well, and would likely be seen by the overlay.

There seems to be no way to expose the internal dispatch headers to the Swift side of things. There is a DispatchPrivate module but it won’t import during the build. So, I’ve used a @_silgen_name to pick up a setter function which isn’t ideal but at least it no longer affects the public api.

MadCoder · 2017-05-26T18:21:23Z

src/queue.c

@@ -885,6 +889,10 @@ _libdispatch_tsd_cleanup(void *ctx)
 	_tsd_call_cleanup(dispatch_voucher_key, _voucher_thread_cleanup);
 	_tsd_call_cleanup(dispatch_deferred_items_key,
 			_dispatch_deferred_items_cleanup);
+#ifdef __ANDROID__
+	if (_dispatch_thread_detach_callback)


If it was like this before, sorry to have missed it, please add brackets.

MadCoder · 2017-05-27T00:05:08Z

src/queue.c

@@ -861,6 +861,14 @@ gettid(void)
 		if ((f) && tsd->k) ((void(*)(void*))(f))(tsd->k); \
 	} while (0)

+#ifdef __ANDROID__
+static void (*_dispatch_thread_detach_callback)(void);


if it's static then the overlay which is a dynamic library can't pick it up.

@das suggested to me the right place is to export the symbol is from private/queue_private.h or private/private.h and also to use silgen like you did.

Global symbol is now a function _dispatch_set_detach_callback_ptr as it doesn’t seem possible to express a global var in a silgen. I’ve moved it's declaration into queue_private.h.

MadCoder · 2017-05-28T03:53:22Z

src/queue.c

+static void (*_dispatch_thread_detach_callback)(void);
+
+void
+*(*_dispatch_thread_detach_callback_ptr(void))(void) {


this is hard to read, also exposing a pointer to a function pointer is really weird. do you really need the getter? it feels like you only want to install it, I would have gone for:

void _dispatch_install_thread_detach_callback(dispatch_function_t cb) { if (os_atomic_xchg(&_dispatch_thread_detach_callback, cb, relaxed)) { DISPATCH_CLIENT_CRASH(0, "Installing a thread detach callback twice"); } }

MadCoder · 2017-05-28T03:56:29Z

src/swift/Queue.swift

+	@_silgen_name("_dispatch_thread_detach_callback_ptr")
+	private static func _dispatch_thread_detach_callback_ptr() -> UnsafeMutablePointer<(@convention(c) () -> Void)?>
+
+	public static var threadDetachCallback: (@convention(c) () -> Void)? {


I think only a one-way setter should be exposed. having a getter makes the interface un-necessarily ugly.

Thanks for the potiners. It helps me zero in on something you’re happy with. Changed pushed.

johnno1962 · 2017-06-01T13:43:28Z

@MadCoder Any changes you’d like on the latest version?

johnno1962 · 2017-06-20T22:31:06Z

Hi @MadCoder, any chance we could arrive at something that is agreeable to you that we could merge? I’d liek to tie the api down so I can release a Swift/Andoird precompiled toolchain. While it clutters the dispatch api a tad it is pretty much a requirement for apps mixing Java and Swift that use dispatch queues.

MadCoder · 2017-06-20T22:36:51Z

Sorry I had forgotten about this pull request, the latest changes looks good, please squash it in a single commit so that I can merge.

johnno1962 · 2017-06-20T22:38:18Z

Can’t you do that with a button in github as you merge for me? I can look at it but my git skills are very limited!

Thread detach hook for Java JNI on Android II Thread detach hook for Java JNI on Android III Renamed for consistency Revert public API change Move to queue_private.h Implemented getter Single entry point Setter only

…-libdispatch

johnno1962 · 2017-06-20T22:53:15Z

Bah, tried a rebase and there are now 11 commits :( Changes are still correct. Can you squish as you merge please?

CodaFi · 2017-06-20T22:55:19Z

Try

git pull --rebase upstream master followed by git rebase -i to interactively squash.

johnno1962 · 2017-06-20T23:21:19Z

Thanks but I’m afraid that hasn’t worked.. conflicts etc locally now. Can you not squish as you merge? Otherwise I’ll have to start a new clone, remake the changes retest and resubmit.

johnno1962 · 2017-06-21T00:23:25Z

I’ve resubmitted this as PR #259 after remaking the changes to a clean clone of up to date repo.

Thread detach hook for Java JNI on Android

c3c0bef

MadCoder suggested changes May 23, 2017

View reviewed changes

johnno1962 added 3 commits May 23, 2017 07:55

Thread detach hook for Java JNI on Android II

96bc6b6

Thread detach hook for Java JNI on Android III

b6003ed

Renamed for consistency

8bf5bcf

MadCoder suggested changes May 26, 2017

View reviewed changes

Revert public API change

bd54618

MadCoder suggested changes May 27, 2017

View reviewed changes

johnno1962 added 3 commits May 27, 2017 07:28

Move to queue_private.h

4be52da

Implemented getter

10ea8cf

Single entry point

12163ba

MadCoder suggested changes May 28, 2017

View reviewed changes

Setter only

0a57226

johnno1962 added 2 commits June 20, 2017 23:42

Thread detach hook for Java JNI on Android

39eddf2

Thread detach hook for Java JNI on Android II Thread detach hook for Java JNI on Android III Renamed for consistency Revert public API change Move to queue_private.h Implemented getter Single entry point Setter only

Merge branch 'master' of https://github.com/Concoction/swift-corelibs…

41772bd

…-libdispatch

johnno1962 closed this Jun 21, 2017

Thread detach hook for Java JNI on Android #250

Thread detach hook for Java JNI on Android #250

Uh oh!

Conversation

johnno1962 commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnno1962 May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnno1962 May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnno1962 commented Jun 1, 2017

Uh oh!

johnno1962 commented Jun 20, 2017

Uh oh!

MadCoder commented Jun 20, 2017

Uh oh!

johnno1962 commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnno1962 commented Jun 20, 2017

Uh oh!

CodaFi commented Jun 20, 2017

Uh oh!

johnno1962 commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnno1962 commented Jun 21, 2017

Uh oh!

Uh oh!

johnno1962 commented May 23, 2017 •

edited

Loading

johnno1962 May 26, 2017 •

edited

Loading

johnno1962 May 27, 2017 •

edited

Loading

johnno1962 commented Jun 20, 2017 •

edited

Loading

johnno1962 commented Jun 20, 2017 •

edited

Loading